Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update SQL and Oracle module docs regarding Oracle DSNs #37590

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

chrisberkhout
Copy link
Contributor

@chrisberkhout chrisberkhout commented Jan 10, 2024

Proposed commit message

Update SQL and Oracle module docs regarding Oracle DSNs (#37590)

- Add 'oracle://' URL format.
- Add note about encoding of special characters in URLs.
- Align the SQL module and the Oracle module documentation.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

How to test this PR locally

The SQL and Oracle modules both use the GO DRiver for ORacle DB to parse Oracle DSNs. I tested the parsing of the DSN formats with the following script.

DSN parsing script

Script:

package main

import (
	"fmt"

	"github.com/godror/godror"
)

func main() {
	params, err := godror.ParseDSN("oracle://user:pa%20ss@0.0.0.0:1521/ORCLPDB1.localdomain?sysdba=1") // ok
	// params, err := godror.ParseDSN("user/pa%20ss@0.0.0.0:1521/ORCLPDB1.localdomain?sysdba=1") // not ok
	// params, err := godror.ParseDSN("user/pass@0.0.0.0:1521/ORCLPDB1.localdomain?sysdba=1") // ok
	// params, err := godror.ParseDSN("user/password@0.0.0.0:1521/ORCLPDB1.localdomain as sysdba") // ok
	// params, err := godror.ParseDSN("user=\"user\" password=\"pass\" connectString=\"0.0.0.0:1521/ORCLPDB1.localdomain\"") // ok
	// params, err := godror.ParseDSN("user=\"user\" password=\"pass\\\\word\" connectString=\"host:port/service_name\" sysdba=true") // ok

	if err != nil {
		fmt.Println(err)
	} else {
		fmt.Println(params)
		fmt.Println(params.Password.Secret())
	}
}

Output:

user=user password=SECRET-*** connectString=0.0.0.0:1521/ORCLPDB1.localdomain
configDir= connectionClass= enableEvents=0 externalAuth=0 heterogeneousPool=0
libDir= noTimezoneCheck=0 poolIncrement=1 poolMaxSessions=1000 poolMinSessions=1
poolSessionMaxLifetime=1h0m0s poolSessionTimeout=5m0s poolWaitTimeout=30s
prelim=0 standaloneConnection=0 sysasm=0 sysdba=1 sysoper=0 timezone=
pa ss

Related issues

@chrisberkhout chrisberkhout requested a review from a team as a code owner January 10, 2024 10:22
@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 10, 2024
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 10, 2024
Copy link
Contributor

mergify bot commented Jan 10, 2024

⚠️ The sha of the head commit of this PR conflicts with #37582. Mergify cannot evaluate rules on this PR. ⚠️

@chrisberkhout chrisberkhout changed the title Sql oracle dsn formats Update SQL input documentation regarding Oracle DSNs Jan 10, 2024
Copy link
Contributor

mergify bot commented Jan 10, 2024

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @chrisberkhout? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v8./d.0 is the label to automatically backport to the 8./d branch. /d is the digit

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2024-01-10T10:23:14.086+0000

  • Duration: 7 min 33 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@chrisberkhout chrisberkhout deleted the sql-oracle-dsn-formats branch January 10, 2024 10:34
@elasticmachine
Copy link
Collaborator

💔 Build Failed

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 5 min 59 sec

Steps errors 1

Expand to view the steps failures

Shell Script
  • Took 0 min 0 sec . View more details here
  • Description: git diff --name-only origin/main...bdf4017f9456e51eac7935290740d8dd830c80ea > git-diff.txt

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@chrisberkhout chrisberkhout restored the sql-oracle-dsn-formats branch January 10, 2024 10:35
@chrisberkhout chrisberkhout reopened this Jan 10, 2024
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 8 min 3 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@agithomas agithomas self-requested a review January 10, 2024 10:55
Copy link
Member

@ebeahan ebeahan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Looks like this also needs reviewed from @elastic/obs-infraobs-integrations as codeowners.

metricbeat/docs/modules/sql.asciidoc Outdated Show resolved Hide resolved
metricbeat/docs/modules/sql.asciidoc Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Collaborator

❕ Build Aborted

Either there was a build timeout or someone aborted the build.

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Duration: 7 min 37 sec

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 9 min 20 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 8 min 28 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@lalit-satapathy
Copy link
Contributor

My understanding of the current usage is:

  • If old style is used: password will need URL encoding.
  • If DSN style is used: URL encoding is not needed. I don't think it requires a URL encoding, since password is provided directly.

Screenshot 2024-01-24 at 10 40 17 AM

I do agree some explicit clarification is needed in the current wordings.

@chrisberkhout chrisberkhout requested review from a team as code owners January 24, 2024 14:30
@chrisberkhout chrisberkhout changed the title Update SQL input documentation regarding Oracle DSNs Update SQL and Oracle module docs regarding Oracle DSNs Jan 24, 2024
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Duration: 9 min 4 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@chrisberkhout
Copy link
Contributor Author

I've done the following:

  • Accepted the changes from @shmsr.
  • Clarified the difference between "An old-style Oracle connection string" and "DSN configuration as a URL" (since the former won't accept URL encoding).
  • After @lalit-satapathy's comment, I expanded this change to cover the Oracle module documentation (shown in his screeshot) as well as the SQL module documentation. I confirmed that they both use the same DSN parsing library and aligned the overlapping content. Both now recommend DSN formats.

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2024-01-25T11:01:03.639+0000

  • Duration: 8 min 14 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2024-01-26T14:11:37.511+0000

  • Duration: 8 min 10 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2024-01-30T09:28:33.543+0000

  • Duration: 8 min 24 sec

❕ Flaky test report

No test was executed to be analysed.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@chrisberkhout chrisberkhout merged commit 5a9613e into elastic:main Jan 30, 2024
21 checks passed
@chrisberkhout chrisberkhout deleted the sql-oracle-dsn-formats branch January 30, 2024 09:55
Scholar-Li pushed a commit to Scholar-Li/beats that referenced this pull request Feb 5, 2024
- Add 'oracle://' URL format.
- Add note about encoding of special characters in URLs.
- Align the SQL module and the Oracle module documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants